Skip to content
/ rector-src Public

[type-declarations] Open isset check in StrictArrayParamDimFetchRectorTest, move to last position in type-declaration level#7388

Merged
TomasVotruba merged 1 commit intomainfrom
tv-array-param-from-isset-dim-fetch
Oct 1, 2025
Merged

[type-declarations] Open isset check in StrictArrayParamDimFetchRectorTest, move to last position in type-declaration level#7388
TomasVotruba merged 1 commit intomainfrom
tv-array-param-from-isset-dim-fetch

Conversation

@TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Oct 1, 2025 •
edited
Loading

In one project I've noticed there is no array filled where obviously should be one.
Single isset might be triggered on null and key, but maybe more useful cases are ignored then. Let's open up this a bit and test in the wild, if brings more value 👍

@TomasVotruba TomasVotruba force-pushed the tv-array-param-from-isset-dim-fetch branch from 26d22aa to f119912 Compare October 1, 2025 07:52
samsonasik
samsonasik requested changes Oct 1, 2025
View reviewed changes
Copy link
Member

@samsonasik samsonasik left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is rule StrictArrayParamDimFetchRector rule for it, for 0 index without context, it can be string, which maybe not array on purpose, see

rector-src/rules/TypeDeclaration/Rector/ClassMethod/StrictArrayParamDimFetchRector.php

Lines 173 to 177 in edadebb

// skip integer in possibly string type as string can be accessed via int
$dimType = $this->getType($node->dim);
if ($dimType->isInteger()->yes() && $variableType->isString()->maybe()) {
return null;
}

see https://3v4l.org/8h8nm

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Oct 1, 2025 •
edited
Loading

I could not find this one, thanks

I see lot of missed array types just to avoid string access. Will make this condition less strict to improve auto fill 👍

@samsonasik
Copy link
Member

samsonasik commented Oct 1, 2025

Then that's need on very last on level set list with Int_ key, as risky, or just not register in the set list, user can manually use this rule when needed.

Also, still needs ParentClassMethodTypeOverrideGuard->hasParentClassMethod() check still needed.

@TomasVotruba TomasVotruba force-pushed the tv-array-param-from-isset-dim-fetch branch 4 times, most recently from 5c1b4b8 to a5d9773 Compare October 1, 2025 09:23
@TomasVotruba TomasVotruba changed the title [type-declarations] Add AddClassMethodParamArrayWhereDimFetchRector [type-declarations] Open isset check in StrictArrayParamDimFetchRectorTest, move to last position in type-declaration level Oct 1, 2025
@TomasVotruba TomasVotruba force-pushed the tv-array-param-from-isset-dim-fetch branch from a5d9773 to b2dd4a7 Compare October 1, 2025 09:25
@TomasVotruba
[type-declarations] Open isset check in StrictArrayParamDimFetchRecto…
88a4185 
…rTest, move to last position in type-declaration level
@TomasVotruba TomasVotruba force-pushed the tv-array-param-from-isset-dim-fetch branch from 2bc2bc4 to 88a4185 Compare October 1, 2025 09:27
staabm
staabm reviewed Oct 1, 2025
View reviewed changes
...Rector/ClassMethod/StrictArrayParamDimFetchRector/Fixture/cover_isset_with_dim_fetch.php.inc

final class CoverIssetWithDimFetch
{
public function run(array $param, $item): bool
Copy link
Contributor

@staabm staabm Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case $param could be anything which implements ArrayAccess not just the rare case of string-offset access.

in case you "wrongly guess" the array type a call with a ArrayAccess object will produce a fatal error at runtime - see https://3v4l.org/csROV

Copy link
Member Author

@TomasVotruba TomasVotruba Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it's a choice of practice over risk of few edge cases. We apply same logic to other rules too, to ease manual daunting work on project upgrades.

If your codebase uses any type in this case, exclude the rule.

Copy link
Member Author

@TomasVotruba TomasVotruba Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it's a choice of practice over risk of few edge cases. We apply same logic to other rules too, to ease manual daunting work on project upgrades.

If your codebase uses any type in this case, exclude the rule.

@TomasVotruba TomasVotruba merged commit 7a73e88 into main Oct 1, 2025
49 of 50 checks passed
@TomasVotruba TomasVotruba deleted the tv-array-param-from-isset-dim-fetch branch October 1, 2025 15:57
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2026

This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@samsonasik samsonasik samsonasik requested changes

+1 more reviewer

@staabm staabm staabm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TomasVotruba @samsonasik @staabm